Implement PCZT support #440

str4d · 2024-11-27T10:03:01Z

No description provided.

str4d · 2024-11-27T10:12:35Z

src/pczt.rs

+    ///
+    /// - This is chosen by the Constructor.
+    /// - This is required by the IO Finalizer, and is cleared by it once used.
+    /// - Signers MUST reject PCZTs that contain `dummy_sk` values.


I both named this dummy_sk and added a "MUST reject" here because I do not want PCZTs to get serialized with non-dummy spending keys.

I stored the dummy note's sk instead of the smaller-scoped ask, because in Orchard we impose validity requirements at parse time of sk (specifically to ensure that a valid ivk is produced), and we already had APIs for handling that parsing here. If desired this can be scoped down to dummy_ask, but given that (again) this field is only for dummy notes, I think sk should be fine here.

str4d · 2024-11-27T10:14:09Z

src/pczt/parse.rs

+    pub fn parse(
+        actions: Vec<Action>,
+        flags: u8,
+        value_sum: (u64, bool),


I decided on this serialization format for ValueSum (rather than e.g. i128) because it has no edge cases; all values are valid.

nuttycom

utACK e7a2945 with minor questions / naming nits. Overall this looks excellent.

src/builder.rs

src/pczt.rs

nuttycom · 2024-11-27T15:51:57Z

src/pczt.rs

+        let (anchor, merkle_path) = {
+            let cmx: ExtractedNoteCommitment = note.commitment().into();
+            let leaf = MerkleHashOrchard::from_cmx(&cmx);
+            let mut tree = BridgeTree::<MerkleHashOrchard, u32, 32>::new(100);


TODO: since BridgeTree is no longer being maintained, we should move away from using it.

nuttycom · 2024-11-27T18:06:20Z

src/pczt/tx_extractor.rs

+
+/// Authorizing data for a bundle of actions that is just missing a binding signature.
+#[derive(Debug)]
+pub struct Unbound {


nit: this name isn't particularly intuitive to me, but I don't have a better suggestion; maybe BindingSigInputs or something?

The name appears primarily in type signatures, i.e. Bundle<Unbound, ZatBalance>; I chose it to make sense there (same as Authorized / Unauthorized / EffectsOnly).

src/pczt/tx_extractor.rs

src/pczt/io_finalizer.rs

src/pczt.rs

src/pczt/tx_extractor.rs

src/pczt/parse.rs

src/pczt/io_finalizer.rs

str4d · 2024-12-03T11:41:45Z

Force-pushed to address comments and add missing serialization APIs.

str4d · 2024-12-03T11:47:16Z

Force-pushed to address another comment I missed.

str4d · 2024-12-03T11:50:56Z

Force-pushed to address another comment I missed, and fix clippy lints.

src/builder.rs

daira · 2024-12-03T19:59:40Z

src/pczt.rs

+    /// - `ephemeral_key`
+    /// - `enc_ciphertext`
+    /// - `out_ciphertext`
+    pub(crate) encrypted_note: TransmittedNoteCiphertext,


This is going to be transaction version-dependent, e.g. the length will change in NU6 due to ZIP 231 and also ZSAs. So do Output and TransmittedNoteCiphertext need to be version-dependent types, or is it sufficient for TransmittedNoteCiphertext to contain a vector that is dynamically of the correct length?

This is the parsed typed type, so it can be whatever we want. The PCZT encoding uses Vec<u8> for both ciphertexts in anticipation of NU7.

nuttycom

utACK cb404f8 with nonblocking question about how to handle TransmittedNoteCiphertext in the future.

src/pczt.rs

Co-authored-by: Daira-Emma Hopwood <[email protected]> Co-authored-by: Kris Nuttycombe <[email protected]>

nuttycom

re-utACK d132b5b

src/pczt/parse.rs

daira

utACK

daira

Doc-only self-utACK 5b6cc68

str4d marked this pull request as draft November 27, 2024 10:03

str4d commented Nov 27, 2024

View reviewed changes

nuttycom previously approved these changes Nov 27, 2024

View reviewed changes